feat: reduce block interval poc #265
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
will-2012
force-pushed
the
reduce_block_interval_poc
branch
4 times, most recently
from
6f6c827 to
01d6315
Compare
will-2012
force-pushed
the
reduce_block_interval_poc
branch
21 times, most recently
from
baa5440 to
bf3372a
Compare
will-2012
force-pushed
the
reduce_block_interval_poc
branch
5 times, most recently
from
7da7fa8 to
bff700d
Compare
will-2012
force-pushed
the
reduce_block_interval_poc
branch
from
bff700d to
8520a2f
Compare
RenRick
reviewed
Mar 3, 2025
RenRick
reviewed
Mar 3, 2025
RenRick
reviewed
Mar 3, 2025
flywukong
force-pushed
the
reduce_block_interval_poc
branch
from
95b03d8 to
d1eb66f
Compare
flywukong
force-pushed
the
reduce_block_interval_poc
branch
from
3c284e5 to
a717b8c
Compare
will-2012
commented
Mar 4, 2025
op-service/eth/heads.go
Outdated
| @@ -43,11 +45,18 @@ func WatchHeadChanges(ctx context.Context, src NewHeadSource, fn HeadSignalFn) ( | |||
| for { | |||
| select { | |||
| case header := <-headChanges: | |||
| var mTime uint64 | |||
| if header.MixDigest == (common.Hash{}) { | |||
| mTime = header.Time | |||
There was a problem hiding this comment.
Why is mTime not initialized to 0?
will-2012
commented
Mar 4, 2025
will-2012
commented
Mar 4, 2025
flywukong
force-pushed
the
reduce_block_interval_poc
branch
from
a20bd64 to
618dbe0
Compare
Co-authored-by: 2020xibao <[email protected]>
will-2012
commented
Mar 4, 2025
op-node/rollup/driver/sequencer.go
Outdated
| d.CancelBuildingBlock(ctx) | ||
| return nil, err | ||
| } else if errors.Is(err, derive.ErrTemporary) { | ||
| d.log.Error("sequencer failed temporarily to seal new block", "err", err) | ||
| d.nextAction = d.timeNow().Add(time.Second) | ||
| d.nextAction = d.timeNow().Add(backoffTime) |
There was a problem hiding this comment.
The original semantics was to backoff for one second, not necessarily the interval of the block. Why did this change the semantics?
will-2012
commented
Mar 4, 2025
op-node/rollup/driver/sequencer.go
Outdated
| // We don't explicitly cancel block building jobs upon temporary errors: we may still finish the block. | ||
| // Any unfinished block building work eventually times out, and will be cleaned up that way. | ||
| } else { | ||
| d.log.Error("sequencer failed to seal block with unclassified error", "err", err) | ||
| d.nextAction = d.timeNow().Add(time.Second) | ||
| d.nextAction = d.timeNow().Add(backoffTime) |
RenRick
reviewed
Mar 5, 2025
RenRick
reviewed
Mar 5, 2025
RenRick
reviewed
Mar 5, 2025
Co-authored-by: 2020xibao <[email protected]>
Co-authored-by: 2020xibao <[email protected]>
joeylichang
reviewed
Mar 7, 2025
| @@ -362,11 +362,18 @@ func (s *channelManager) AddL2Block(block *types.Block) error { | |||
| } | |||
|
|
|||
| func l2BlockRefFromBlockAndL1Info(block *types.Block, l1info *derive.L1BlockInfo) eth.L2BlockRef { | |||
| milliPart := uint64(0) | |||
There was a problem hiding this comment.
L2BlockRef as the l2BlockRefFromBlockAndL1Info return value, it is only used to mertics, so it is useless for adding the milliPart.
|
It is most important for op-batcher that adda millsecondes into |
joeylichang
approved these changes
Mar 7, 2025
| PrevRandao: eth.Bytes32(l1Info.MixDigest()), | ||
| SuggestedFeeRecipient: predeploys.SequencerFeeVaultAddr, | ||
| Transactions: txs, | ||
| NoTxPool: true, | ||
| GasLimit: (*eth.Uint64Quantity)(&sysConfig.GasLimit), | ||
| Withdrawals: withdrawals, | ||
| ParentBeaconBlockRoot: parentBeaconRoot, | ||
| }, nil | ||
| } | ||
| pa.SetMillisecondTimestamp(nextL2MilliTime) |
There was a problem hiding this comment.
SetMillisecondTimestamp(common.hash, millseconds) -> set millseconds to first common.hash.
then the return value to init eth.PayloadAttributes in 173 line, may be better.
| @@ -96,7 +96,7 @@ func (bq *BatchQueue) NextBatch(ctx context.Context, parent eth.L2BlockRef) (*Si | |||
| if len(bq.nextSpan) > 0 { | |||
| // There are cached singular batches derived from the span batch. | |||
| // Check if the next cached batch matches the given parent block. | |||
| if bq.nextSpan[0].Timestamp == parent.Time+bq.config.BlockTime { | |||
| if bq.nextSpan[0].Timestamp == parent.MillisecondTimestamp()+bq.config.MillisecondBlockInterval() { | |||
There was a problem hiding this comment.
if the Timestamp is changed to milliseconds for seconds, add one note that it is active after hard fork, otherwise it is seconds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reduce block interval poc.
Notice: This is a draft PR, a work in progress, and not ready for review.
Rationale
TBD
Example
TBD
Changes
Notable changes: